Fix length change omission for StringSubstitutor cyclic detection #115
Conversation
There was a problem hiding this comment.
Sorry, I tried to understand the change here, or how to reproduce it, but failed at both.
I did port a test from StrSubstitutorTest.java, but the test passes before and after this change
This is definitely not ideal. Is there any other way to reproduce the error before the fix? I tried a few different things on master, comparing with the output of your branch, but still couldn't reproduce the error you mentioned.
I believe this to be a trivial change (hence no JIRA ticket). Let me know if otherwise
I think this would still require a change, as it appears to prevent an error you had (and which users may have as well). Trivial changes are more javadocs, style changes, rename a local variable, fix unused imports, etc. Anything that doesn't affect users regarding backward compatibility (binary or behaviour wise), and also don't add any new feature.
|
My apologies!
diff --git a/src/main/java/org/apache/commons/text/StringSubstitutor.java b/src/main/java/org/apache/commons/text/StringSubstitutor.java
index f4b53ee..4680edc 100644
--- a/src/main/java/org/apache/commons/text/StringSubstitutor.java
+++ b/src/main/java/org/apache/commons/text/StringSubstitutor.java
@@ -1330,7 +1330,7 @@ public class StringSubstitutor {
final boolean top = priorVariables == null;
boolean altered = false;
int lengthChange = 0;
- char[] chars = buf.buffer;
+ char[] chars = buf.toCharArray();
int bufEnd = offset + length;
int pos = offset;
while (pos < bufEnd) {
@@ -1346,7 +1346,7 @@ public class StringSubstitutor {
continue;
}
buf.deleteCharAt(pos - 1);
- chars = buf.buffer; // in case buffer was altered
+ chars = buf.toCharArray(); // in case buffer was altered
lengthChange--;
altered = true;
bufEnd--;
@@ -1432,7 +1432,7 @@ public class StringSubstitutor {
pos += change;
bufEnd += change;
lengthChange += change;
- chars = buf.buffer; // in case buffer was altered
+ chars = buf.toCharArray(); // in case buffer was altered
} else if (undefinedVariableException) {
throw new IllegalArgumentException(String.format(
"Cannot resolve variable '%s' (enableSubstitutionInVariables=%s).", varName,
if (priorVariables == null) {
priorVariables = new ArrayList<>();
priorVariables.add(new String(chars, offset, length));
}
|
It's a logic bug that can't manifest in commons-text is due to the reliance on internal Let me know if you still want me to create a JIRA ticket 😄 |
|
I would like feedback from the community on whether or not this should be merged. A failing test would make this a no-brainer of course but I understand this might not be possible here. |
StringSubstitutor#substitutewould throwStringIndexOutOfBoundsExceptionwhen removing a variable start marker ifStringSubstitutorwasn't using the internal representation ofTextStringBuilder#buffer. The local variable,priorVariables, contains unbalanced squirrely parentheses (replacing$${${TEST}}would causepriorVariablesto add${${TEST}}}-- amusingly didn't result in unexpected behavior). I discovered this when swapping all instances ofbuf.buffertobuf.toCharArray(). This PR ensures that both now have equivalent behavior. I was unable to tease out a test for this, as the length is only decremented to that point (but this change in length goes unused, hence the exception fortoCharArray), so no overflows. I did port a test fromStrSubstitutorTest.java, but the test passes before and after this change.I believe this to be a trivial change (hence no JIRA ticket). Let me know if otherwise